Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding Unit Level Epub Export #1977

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Adding Unit Level Epub Export #1977

wants to merge 7 commits into from

Conversation

bgargi
Copy link

@bgargi bgargi commented Jun 5, 2018

Description:
Added feature for exporting a single unit into epub format.
By- Xchange Group BITS Pilani Goa

@kedar2a
Copy link
Contributor

kedar2a commented Jun 5, 2018

👍 for your first pull request (PR).


Adding some suggestion for this PR:

VIEWS/URLS

  • Create new view/url only when there is a new feature/module creation.
  • Possibly try to write code in existing view: export_to_epub.py.
    • This will avoid rewriting of same code.
    • Maintain one copy of code/function, easy to make future changes in logic or policy.
    • Improvise existing function, made it more versatile.

HTML TEMPLATES:

  • Do not write inline css. It is good to start and quick test but not to go in master branch. Either add it in infile or external (always best approach) css/saas file.
  • Check responsiveness of site/page-rendering after adding new CSS.
  • Be careful ( do it only when it's needed) while changing logical conditions within templates.

</a>
</li>
</li>
{% endcomment %}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this code is not needed, remove it.
If needed in near future, add developers note/comments in it.

<li>
<a href="{% url 'course_pages' group_name %}" {% if title == 'course_pages' or title == 'create_course_pages' %}class="selected"{% endif %}><i class="fi-page-filled "></i> {% trans "Activities" %}</a>
</li>
<li>
<a href="{% url 'asset_list' group_name %}" {% if title == 'asset_list' or title == 'asset_detail' or title == 'asset_content_detail'%}class="selected"{% endif %}><i class="fi-folder-lock"></i> {%trans "Assets" %}</a>
</li>
{% if group_object.agency_type != 'School' and "Author" not in group_object_member_of_names_list %}
{% if group_object.agency_type != 'School' %}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While making changes in any of the logical conditions, test it with following use users:

  1. Anonymous
  2. Normal
  3. Group Admin
  4. Super

@@ -62,6 +62,7 @@
url(r'^save_metadata', 'save_metadata', name='save_metadata'),
url(r'^save_interactions', 'save_interactions', name='save_interactions'),
url(r'^export_to_epub/(?P<node_id>[\w-]+)', 'export_to_epub', name='export_to_epub'),
url(r'^export_unit_to_epub','export_unit_to_epub',name='export_unit_to_epub'),#Added by Gargi Balasubramaniam
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not necessary because git keeps details for each and every line:

  • creator
  • timestamp
  • commit no
  • commit msg

pass
return HttpResponseRedirect(reverse('unit_detail', kwargs={'group_id': group_id}))


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try to use the existing method and make it more versatile. Method behavior should be decided by args.

@bgargi
Copy link
Author

bgargi commented Jul 11, 2018

Have also committed changes for adding 2 docs- Export_moodle and Epub_issues(for script issues which cause epub validity issues).

@gnowgi
Copy link
Member

gnowgi commented Oct 25, 2018

@SheetalKashid please test this and let me know the test results. This is an important and useful feature, so should be merged in the master.

@gnowgi gnowgi closed this Oct 25, 2018
@gnowgi gnowgi reopened this Oct 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants